-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix 6805: remove strange groupBy behavior #7252
Fix 6805: remove strange groupBy behavior #7252
Conversation
`groupBy` no longer supports the behavior where inner subscriptions will cause the outer subscription to stay connected after consumer unsubscribes from result. Resolves ReactiveX#6805 BREAKING CHANGE: `groupBy` no longer allows grouped observable subscriptions to stay connected after parent subscription unsubscribed. If you need this behavior, don't unsubscribe from the parent.
+ Stopped exporting `OperatorSubscriber` class from module + Updated `onErrorResumeNext` and a `Subject` test to use `createOperatorSubscriber`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only one comment from the code needs to be updated.
// Capturing a reference to this, because we need a handle to it | ||
// in `createGroupedObservable` below. This is what we use to | ||
// subscribe to our source observable. This sometimes needs to be unsubscribed | ||
// out-of-band with our `subscriber` which is the downstream subscriber, or destination, | ||
// in cases where a user unsubscribes from the main resulting subscription, but | ||
// still has groups from this subscription subscribed and would expect values from it | ||
// Consider: `source.pipe(groupBy(fn), take(2))`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revisit this comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added one additional note about a comment that probably needs revisiting. Other than that LGTM
// Capturing a reference to this, because we need a handle to it | ||
// in `createGroupedObservable` below. This is what we use to | ||
// subscribe to our source observable. This sometimes needs to be unsubscribed | ||
// out-of-band with our `subscriber` which is the downstream subscriber, or destination, | ||
// in cases where a user unsubscribes from the main resulting subscription, but | ||
// still has groups from this subscription subscribed and would expect values from it | ||
// Consider: `source.pipe(groupBy(fn), take(2))`. | ||
const groupBySourceSubscriber = new OperatorSubscriber( | ||
const groupBySourceSubscriber = createOperatorSubscriber( | ||
subscriber, | ||
(value: T) => { | ||
// Because we have to notify all groups of any errors that occur in here, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GH didn't allow me to put a comment in the exact spot lines 189-191 here but I think the comment also needs revisiting as the mentioned reference counting seem to be removed now.
fix(groupBy): will no longer leak inner subscriptions
groupBy
no longer supports the behavior where inner subscriptions will cause the outer subscription to stay connected after consumer unsubscribes from result.Resolves #6805
BREAKING CHANGE:
groupBy
no longer allows grouped observable subscriptions to stay connected after parent subscription unsubscribed. If you need this behavior, don't unsubscribe from the parent.refactor: Remove direct instantiation of
OperatorSubscriber
OperatorSubscriber
class from moduleonErrorResumeNext
and aSubject
test to usecreateOperatorSubscriber
Actual stream of the work is here, if that helps: https://youtu.be/rHgED1U6XDY